Skip to content

fix: OPTIC-945: Improve the bundling strategy and ship a single entrypoint with a common vendor chunk across all libs #6152

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 98 commits into from
Aug 13, 2024

Conversation

bmartel
Copy link
Contributor

@bmartel bmartel commented Jul 30, 2024

PR fulfills these requirements

  • Commit message(s) and PR title follows the format [fix|feat|ci|chore|doc]: TICKET-ID: Short description of change made ex. fix: DEV-XXXX: Removed inconsistent code usage causing intermittent errors
  • Best efforts were made to ensure docs/code are concise and coherent (checked for spelling/grammatical errors, commented out code, debug logs etc.)
  • Self-reviewed and ran all changes on a local instance (for bug fixes/features)

Change has impacts in these area(s)

(check all that apply)

  • Product design
  • Backend (Database)
  • Backend (API)
  • Frontend

Describe the reason for change

This change aligns the cross library component usage with React, such that it can and is now bundling only 1 set of dependencies across all of the code shipped as part of the frontend. The primary driving force of this change is to enable better code bundling strategies, reduce the amount of code shipped, fix our React usage which had become tied to workarounds from the triplicated definitions of React in various bundles and align the codebase to being able to start extracting common components and utils to a shared space. This PR now allows us to adjust, measure and update the bundling strategy further than proposed here, and will also allow a component by component reduction in the codebase. The end goal being, to ship less code to the end user more granularly with improved performance and consistency.

Functionally and stylistically this should remain exactly as previous. The only difference is in how we are bundling the code, and with which parts are put in specific bundles to retain a better longer term caching strategy between releases.

To achieve this single bundle we had to opt to de-duplicate the components which conflicted in css definitions, which ultimately based on the findings of the current state was more tenable to hardcode the prefix of a given component in conflict as a postfix of the block. This is backwards compatible and also addresses the issue in unifying our build.

Notes to reviewers

Explicitly this change has a direct dependency on the decommissioning and removal of the FF fflag_fix_front_lsdv_4620_memory_leaks_100723_short. The reason is, we are now only using a single React and this change severs React fibers which are still in use by the remainder of page and does not allow React to properly flush pendingContext as a result, which causes errors and functionality to break. As part of this change we are going to be rolling this out with the FF turned off, so that it operates correctly for everyone. I tested this branch, and there are no memory leaks with this FF turned off, which aligns with the thinking that the original need to have an experimental cleanup was due to our React usage which caused things to linger in memory without ability for the browser to ever garbage collect it.

What libraries were added/updated?

None. Webpack config was the largest difference here, same version is used for all libs and plugins at this time.

Does this change affect performance?

It can increase performance due to the removal of many duplicated portions of code, but ultimately the performance gain initially here is in the more deterministic cache strategy around the code bundles shipped being more resistant to unnecessary change. This means less code being reloaded between versions shipped, and stronger cache lifetimes between releases which improves load times in any given page by shipping less code over time for all users.

Does this change affect security?

No.

What alternative approaches were there?

The one other alternative to the particular solution here was to fix the CSS prefixing collisions with a modified webpack config for the css_prefix value injected in css/scss/styl files during processing. This was attempted and blocked by the fact that in a given build, if the file which would be produced is a culmination of multiple other css from multiple potential sources (LS App, Editor, DM) it only could be made to apply a single css prefix holistically to the file output, and didn't allow for proper distinguishing between which files had sourced the particular css chunk. This ultimately meant that it was non trivial to achieve this level of change with Webpack, and decided to not pursue any further.

What feature flags were used to cover this change?

None, it wasn't possible to entertain many builds to possibly FF copies all over the place of similar files to retain the previous and the new. This would only further complicate the situation and make it even harder to move forward.

Does this PR introduce a breaking change?

(check only one)

  • Yes, and covered entirely by feature flag(s)
  • Yes, and covered partially by feature flag(s)
  • No
  • Not sure (briefly explain the situation below)

Which logical domain(s) does this change affect?

All.

bmartel and others added 30 commits July 25, 2024 13:26
@yyassi-heartex yyassi-heartex requested a review from a team as a code owner August 7, 2024 22:27
@yyassi-heartex
Copy link
Contributor

yyassi-heartex commented Aug 7, 2024

/docker build

Workflow run
Docker image was pushed with the tag 20240807.233130-fb-optic-945-ae4739667

@yyassi-heartex
Copy link
Contributor

yyassi-heartex commented Aug 8, 2024

/docker build

Workflow run
Docker image was pushed with the tag 20240808.121759-fb-optic-945-36ea7280c

@yyassi-heartex
Copy link
Contributor

yyassi-heartex commented Aug 8, 2024

/docker build

Workflow run
Docker image was pushed with the tag 20240808.124951-fb-optic-945-d428c41db

@yyassi-heartex
Copy link
Contributor

yyassi-heartex commented Aug 8, 2024

/git merge develop

Workflow run
Successfully merged: 5 files changed, 8 insertions(+), 114 deletions(-)

@yyassi-heartex
Copy link
Contributor

yyassi-heartex commented Aug 9, 2024

/docker build

Workflow run
Docker image was pushed with the tag 20240809.175843-fb-optic-945-db8ac02a0

@yyassi-heartex yyassi-heartex merged commit 8105d62 into develop Aug 13, 2024
40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants